SOF msi enable#27
Conversation
This patch tries to enable msi for sof hda audio driver. If it fails (not supported by HW/BIOS), it will use the legacy interrupt mode. Signed-off-by: Libin Yang <libin.yang@intel.com>
This patch tries to enable msi for sof byt pci audio driver. If it fails (not supported by HW/BIOS), it will use the legacy interrupt mode. Signed-off-by: Libin Yang <libin.yang@intel.com>
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Minor nitpicks.
I am also not really hot on code that was never tested, e.g. for BYT/PCI this would require Edison/Tangier to work.
| * register our IRQ | ||
| * let's try to enable msi firstly | ||
| * if it fails, use legacy interrupt mode | ||
| * TODO: support interrupt mode selection with kernel parameter |
There was a problem hiding this comment.
what does it take to add this? the kernel parameter doesn't seem very complicated, but I don't know what the multiple vectors mean in terms of functionality/schedule?
There was a problem hiding this comment.
kernel parameter is not a must feature. It offers a choice to user to change the default behavior. There is no requirement so far and I didn't implement it. I think we can implement it only when we have such requirement, such as MSI is supported but doesn't work. In this case we need a quirk to let user set the default mode is legacy interrupt mode.
Multiple vectors is a feature of MSI, with this feature, device can offer multiple irq number. I checked on apl, and it only uses 1. To enable multiple msi vectors, the code will be a little complicated. I don't have platform to test multiple msi vectors, so I enable the single multiple currently.
| sdev->ipc_irq = pci->irq; | ||
| } else { | ||
| dev_info(sdev->dev, "use msi interrupt mode\n"); | ||
| sdev->hda->irq = pci_irq_vector(pci, 0); |
There was a problem hiding this comment.
can you change this to avoid two calls, pci_irq_vector isn't a small function:
int pci_irq_vector(struct pci_dev *dev, unsigned int nr)
{
if (dev->msix_enabled) {
struct msi_desc *entry;
int i = 0;
for_each_pci_msi_entry(entry, dev) {
if (i == nr)
return entry->irq;
i++;
}
WARN_ON_ONCE(1);
return -EINVAL;
}
if (dev->msi_enabled) {
struct msi_desc *entry = first_pci_msi_entry(dev);
if (WARN_ON_ONCE(nr >= entry->nvec_used))
return -EINVAL;
} else {
if (WARN_ON_ONCE(nr > 0))
return -EINVAL;
}
return dev->irq + nr;
}
EXPORT_SYMBOL(pci_irq_vector);
There was a problem hiding this comment.
It makes sense. I will do it :)
|
Yes, me too. I think we can merge the patch for byt later after test and merge the patch for hda firstly? |
2 participants
Based on Liam's comments, remove the msi_enabled flag.
Tested on CNL, APL and BYT.
On out BYT platform, it doesn't support MSI (not pci device). It will use the legacy interrupt mode.